fix: three cadence/update bugs from the PR#1421 review (+2 found in review) - #1423
Merged
Conversation
Part 1 - Emcee and BlackJAX NUTS crashed on a real cadence, the same defect PR#1421 fixed for MultiStart: emcee's sample() does range(iterations) with no cast, and jax.random.split rejects a float. Both verified by probing the callee rather than reading the call shape. The plan said fix the producer (the float() coercion in AbstractSearch.__init__), on the grounds that Python ints hold 1e99 fine. Rejected after auditing the consumers: int(1e99) is a 99-digit integer, and writing that into every saved search.json in place of a readable inf-like sentinel is a bad trade. Instead the conversion moves up to one shared, validated AbstractSearch._steps_until_full_update, so it is derived once rather than re-derived (and forgotten) per search - which is exactly how this class of bug shipped three times. MultiStart's local _steps_in_chunk is folded into it. Part 2 - MultiStart emitted during_analysis=False on its last chunk while start_resume_fit emits the final update unconditionally anyway, so the whole final pass (sample output, latents, visualization, profiling) ran twice on every search. Emcee, BFGS and Nautilus all pass True unconditionally; MultiStart was the outlier. Now it matches them. Part 3 - a resumed run inherited the previous run's stop_reason, so raising n_steps to extend a finished search left a stale max_steps in every intermediate checkpoint and a still-running search reported itself finished. Cleared on entry, with converged preserved because the loop guard uses it to refuse resuming a converged search. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three findings, all confirmed before acting. 1. The Part 2 fix was cosmetic. Flipping during_analysis to True does not deduplicate the final pass: SearchUpdater.update rebuilds the samples, recomputes the summary and re-runs likelihood profiling on every call regardless of the flag, and the visualize gate keys off paths.is_complete, which is not written until after _fit returns. The in-loop update is now skipped outright at a terminal boundary. On the default single-chunk cadence _fit emits no update at all, leaving exactly one final pass. 2. Zeus was NOT safe, just not crashing. It casts iterations internally, but PyAutoFit adds the uncast float to total_iterations, so a fractional cadence drifts the bookkeeping away from the samples zeus actually drew (nsteps=100, cadence=50.9 finishes with 99). BFGS also bypassed the validation the helper advertises for maxiter. Both now use the shared helper, so every chunked search derives its chunk one way. This supersedes the issue's "do not touch zeus/bfgs" note, which rested on my own earlier finding that they were fine. 3. The falsy-cadence branch reintroduced the silent behaviour the validation exists to remove: a stored 0 meant "never checkpoint". 1e99 is already that sentinel and needs no special case, so 0 is a misconfiguration - reachable through the HPC override, which assigns the config value with no "or" fallback - and now raises. Also fixes the test guards the review showed were brittle or tautological: BlackJAX, Zeus and BFGS added to the wiring guard; the loop-guard assertion now pins the while line rather than being satisfied by the clearing if; source assertions normalise whitespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second review pass confirmed the production fixes (it ran real JAX fits: a one-chunk run now emits zero in-loop updates and exactly one final update; a two-chunk run emits [True, False]). What it did not accept were the regression tests: source-level string assertions can pass while a semantically-equivalent regression is reintroduced - assigning is_final False, or adding a second unconditional update after the guarded one. So extract the two rules the loop was applying inline into named seams - _is_final_boundary and _stop_reason_on_resume - and test those directly and exhaustively, the same move that made the cadence fix testable without JAX. The source assertion shrinks to a wiring guard over the call sites, with the behaviour pinned by the parametrised tests instead. Known residual, accepted rather than papered over: the cross-search wiring guard still cannot prove a search *uses* the value the helper returns, so a contrived regression that calls the helper and discards the result would pass it. Closing that needs the searches' _fit bodies, which need jax/optax/emcee and are out of scope for the NumPy-only library suite; it is covered where those bodies actually run, in the workspace test repos. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1422.
Three bugs surfaced by the adversarial review of #1420 / PR#1421, fixed in one PR
at the human's request. Two more were found during this PR's own review and are
fixed here too.
Summary
Part 1 — Emcee and BlackJAX NUTS crashed on a real
iterations_per_full_updatecadence, the same defect PR#1421 fixed for MultiStart.
emcee'ssample()doesrange(iterations)with no cast of its own;jax.random.splitrejects a float.Both verified by probing the callee, not by reading the call shape.
The issue plan said to fix the producer (the
float()coercion atabstract_search.py:219), on the grounds that Python ints hold1e99fine.Rejected after auditing the consumers:
int(1e99)is a 99-digit integer,and writing that into every saved
search.jsonin place of a readable inf-likesentinel is a bad trade for a conversion each caller can do at the point of use.
Instead the conversion moves up, into one shared, validated
AbstractSearch._steps_until_full_update. Deriving it once beats re-deriving itper search, because re-deriving it is precisely how this bug class shipped three
times.
AbstractMultiStartGradient._steps_in_chunk(added in PR#1421) is foldedinto it.
Part 2 — MultiStart ran the expensive final pass twice.
_fitemittedduring_analysis=Falseon its last chunk whilestart_resume_fitperforms thefinal update unconditionally anyway.
The first attempt here just flipped the flag to
True, matching every siblingsearch. Review showed that was cosmetic:
SearchUpdater.updaterebuilds thesamples, recomputes the summary and re-runs likelihood profiling on every call
regardless of the flag, and the visualization gate keys off
paths.is_complete,which isn't written until after
_fitreturns. The in-loop update is nowskipped outright at a terminal boundary.
Part 3 — a resumed run inherited the previous run's
stop_reason. Raisingn_stepsto extend a finished search left a stale"max_steps"in everyintermediate checkpoint, so a still-running search reported itself finished to
the aggregator and any results inspector. Cleared on entry, with
"converged"preserved because the loop guard uses it to refuse resuming a converged search.
Two extra bugs found by this PR's own review
Zeus was not safe — just not crashing. I had cleared it on #1420 because it
casts internally (
self.nsteps = int(iterations)). But PyAutoFit adds theuncast float to its own
total_iterations(zeus/search.py:261), so afractional cadence drifts the bookkeeping away from the samples zeus actually
drew:
nsteps=100, iterations_per_full_update=50.9runs 50 then 49 steps — 99samples — while the bookkeeping reaches 100. BFGS also bypassed the validation
the helper advertises for
maxiter. Both now use the shared helper. Thissupersedes the "do not touch zeus/bfgs" note on #1422, which rested on my own
earlier, wrong finding.
The falsy-cadence branch reintroduced the silent behaviour the validation
exists to remove: a stored
0meant "never checkpoint".1e99is already thatsentinel and flows through the
minwithout a special case, so0is amisconfiguration — reachable via the HPC override, which assigns the config value
with no
orfallback — and now raises.API Changes
None. Three new private methods (
AbstractSearch._steps_until_full_update/._check_step_count,AbstractMultiStartGradient._is_final_boundary/._stop_reason_on_resume);AbstractMultiStartGradient._steps_in_chunk, privateand added in the previous PR, is removed. No public class, signature, argument,
default or config key changed, and the serialized
iterations_per_full_updatestays the float
1e99— deliberately.Behaviour changes are all corrective:
ValueErrorinstead of hanging, drifting, or silently disabling checkpointing;
and figures/summaries are written once instead of twice;
Testing
pytest test_autofit/→ 1557 passed, 1 skipped.test_autofit/non_linear/search/test_steps_until_full_update.py— theshared cadence seam: a real cadence returns an
intrange()consumes; clampsto the remaining budget; the
1e99default is one chunk; unusable cadence(
0.5,50.9,-5), unusable remaining budget (2.5,0,-10) and astored
0each raise; plus a wiring guard across all five chunked searches._is_final_boundaryand_stop_reason_on_resumeare parametrisedexhaustively, so Parts 2 and 3 are pinned by behaviour, not by source
strings.
_fitbodies needjax/optax/emcee and are exercised in the workspace test repos.
Review
Two adversarial passes with Codex
gpt-5.6-sol(xhigh), per the instruction on#1422.
cosmetic; zeus/bfgs still bypassed the helper; the falsy branch was silent.
one-chunk run produces one checkpoint, zero in-loop updates and exactly one
outer
during_analysis=Falseupdate; a two-chunk run produces[True, False]and two checkpoints. It also confirmed all five helper callers accept the
normal integral-float configs and that no packaged, HPC or test config stores a
value that now raises.
that a semantically-equivalent regression could slip past. Addressed by
extracting the two rules into seams and testing those directly.
Known residual, stated rather than papered over: the cross-search wiring
guard cannot prove a search uses the value the helper returns, so a contrived
regression that calls it and discards the result would pass. Closing that needs
the
_fitbodies, which need jax/optax/emcee — out of scope for the NumPy-onlylibrary suite, and covered where those bodies actually run, in the workspace test
repos.
🤖 Generated with Claude Code